Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add learner_needs field to contentnode API #12763

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

rtibbles
Copy link
Member

Summary

  • Adds learner_needs field to content node API
  • Adds response regularization to ensure that older public APIs have their learner_needs field backfilled to [] to ensure a consistent data structure on the frontend
  • Handle etag being missing from response headers as an edge case
  • Update and add tests for this
  • Flyby - fix flakey assertion for files by explicitly sorting.

References

Fixes #12687
Fixes one of the flaky tests remaining in #8255

Reviewer guidance

Does the learner_needs field properly appear in both internal and public API endpoints now? Do the test changes make sense.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Oct 29, 2024
@ozer550 ozer550 self-requested a review November 12, 2024 06:02
Copy link
Member

@ozer550 ozer550 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything makes sense! The only thing that I was curious about was adding a positive test to check that we are getting learner_needs as expected, will that be helpful?

@rtibbles
Copy link
Member Author

I think what you're asking for (the positive case) is covered here: https://github.com/learningequality/kolibri/pull/12763/files#diff-1f9030c267d0588f9d761b21abb8e5f2095df45e6ae7e440d3011cd0d30b2a4aR339

When we run the test suite against the remote endpoint, the same assertions are run, so these assertions are asserting that the learner needs are being properly returned for both the internal and public APIs.

@rtibbles rtibbles merged commit f2c200c into learningequality:develop Nov 13, 2024
40 checks passed
@rtibbles rtibbles deleted the i_got_what_you_need branch November 13, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the content node API to show resources / What you need
3 participants